-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Relative <Route> and <Link> #4355
Conversation
modules/resolve.js
Outdated
if (typeof location === 'string') { | ||
return resolve(location, base) | ||
} else { | ||
location.pathname = resolve(location.pathname, base) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already using https://github.com/mjackson/resolve-pathname in the history lib. I think it makes sense to re-use it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested out resolve-pathname
by adding a noncompliant
mode to it: resolvePathname(to, from, noncompliant)
. Essentially all that it does is to not pop the filename (https://github.com/mjackson/resolve-pathname/blob/master/modules/index.js#L26) when in that mode. I also added the modified examples from RFC 3986 to test this out. There were a couple of issues:
-
It does not work when
to
includes a hash or search string. I would expectresolvePathname('?x=y', '/a/b', true)
to resolve to/a/b?x=y
, but it is currently including a trailing slash and resolving to/a/b/?x=y
. This also fails for abnormal examples likeg#s/../x
resolving to/a/b/c/d/x
. I was dealing with that by checking for the earliest index of?
or#
, stripping that index to the end of of the string off, resolving, then appending that to the resolved pathname before returning. It is easy to add that functionality toresolvePathname
assuming you want to support that. -
There are some dot and double-dot notation issues. It gets a little bit complicated because this isn't RFC compliant, but I feel that
resolvePathname('..', '/a/b/c/d', true)
should resolve to/a/b/c
while it is resolving to/a/b/c/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not work when to includes a hash or search string.
resolve-pathname
is designed to resolve only the pathname portion of the URL. Leaving out search and hash was intentional. As you say, stripping and re-adding that stuff after the pathname is correctly resolved is the easy part :) I think I'd rather do that in the router codebase somewhere and just let resolve-pathname
do the one thing it's good at.
As for non-compliance with the RFC, I agree with you. Having a <Route path="c">
nested under a <Route path="/a/b">
should match /a/b/c
. However, I think we should still be able to use resolve-pathname
for that by just adding a trailing slash to the parent route's path before passing it in, e.g. resolvePathname(parentMatch.path + '/', path)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a trailing slash should work, not sure why I didn't think of that before.
I think that I will need to add a check so that resolving ..
with /a/b/c
returns /a/b
and not /a/b/
, but otherwise no issues with that comes to mind.
Thanks for another great PR, @pshrmn. We've talked a lot about including support for relative One thing I'd like to make sure we remember to do is to keep support for absolute Also, I think we may be able to leverage the work I've done in https://github.com/mjackson/resolve-pathname. Seems like some of this is duplicating what I've already done there. As far as including better support for relative routes/links in the router, we're very keen to do so. As with everything, just need to be careful we get it right when we do ;) How would you feel about working up |
I can add a test for that. The nice thing about relative <Route path='/a' render={() => (
// this will never be matched.
<Route path='/b/c' component={BC} />
)} /> |
We can actually emit a warning to let people know about that if we wanted to. |
|
Really, I probably should have just modified the existing |
OK, let's try incorporating it directly into |
Just a heads up @pshrmn: everything got moved around in 4349de3. We're following the same "monorepo" design as React itself because we're going to start splitting things out into several different packages. |
@mjackson Before I accidentally rebase and add a thousand files, the |
@pshrmn I always add |
Added relative support to Right now the only thing that I would consider changing is that there is no default { path: '/', url: '', params: {}, isExact: false? } I had thought at first there was an error in Side note: I had added |
That works well with smaller projects, but on something this big (or if there was a larger team), it will result in problems for those who want to contribute and don't have that set up. I would really recommend adding it here. |
Why not just resolve the link in |
Because we need a real |
The browser will resolve relative links in hrefs for you: http://jsbin.com/cugifakufo/edit?html,output What does that have to do with a11y? (Legit question, I'm not an expert in it) |
@timdorr The pathnames in a i.e. const App = () => (
<BrowserRouter>
<div>
<Link to='products'>Products</Link> // resolves to /products
<Route path='products' render={() => (
<div>
<Link to='tables'>Tables</Link> // resolves to /products/tables
<Route path=':item' render={() => (
<Link to='..'>Products</Link> // resolves to /products
)} />
</div>
)} />
</div>
</BrowserRouter>
) |
As @ryanflorence said in #4153 (comment):
This is a tricky problem to solve ;) |
49f568b
to
e88acb2
Compare
The function resolveLocation can be used for resolving location descriptors and strings. Resolving is done with a modified version of RFC 3986's resolution algorithm in which the trailing URL segment is not removed. This allows a <Link> to be resolved relative to the <Route> that is is located in. resolveLocation supports pathnames that include double dot notation (../foo). The function simpleResolve can be used for resolving <Route> and <Switch> pathnames. This simply joins the pathname (iff it is not absolute) to the base pathname using a forward slash.
The parent match is passed to matchPath. match.url is used for resolving instead of math.path to simplify matching (just match a string instead of having to parse params). matchPath will combine its parent's params with its params (favoring its params when there are conflicts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that things are pretty hectic with the beta, but I think that this is in a pretty good place for a review @mjackson (and anyone else).
There appears to be a problem in the tests that I don't run into locally, so I'll need to figure that out, but otherwise everything seems to work for me.
} | ||
|
||
this.unlisten = this.router.listen(() => { | ||
this.router.location = this.context.router.location |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to modify <ExampleRouter>
in order for it to work with relative routes. The <FakeBrowser>
renders everything inside of a pathless <Route>
, so without this relative <Route>
s and <Link>
s would resolve using that <Route>
's match
. This just sets the <ExampleRouter>
's match
to null
. It also has to listen for location changes because any of its children that listen for location changes would be re-rendering via the listener
prior to <ExampleRouter>
's getChildContext
being called on navigation.
@@ -98,7 +104,7 @@ class Route extends React.Component { | |||
|
|||
componentWillReceiveProps(nextProps) { | |||
Object.assign(this.router, { | |||
match: computeMatch(this.router, nextProps) | |||
match: computeMatch(this.context.router, nextProps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to use the parent router
object, not this.router
in order to have access to the parent match
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes me nervous. If it's needed here, it feels like the kind of thing that should be needed more generally. If it's not needed outside the context of this PR, it's probably going to cause a bug.
return { url: pathname, isExact: true, params: {} } | ||
return { url: parent ? parent.url : '/', isExact: true, params: {} } | ||
|
||
path = simpleResolve(path, parent && parent.url ? parent.url : '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve using parent.url
instead of parent.path
for a simpler regexp comparison. Below, the parent's params are assigned to the params
object so that the child can also reference them.
<Route path=':category' render={() => (
<Route path=':id' render={({ params }) => (
console.log(params), // { category: 'lamps', id: 'i<3' }
null
)} />
)} />
const { exact = false, strict = false } = options | ||
|
||
if (!path) | ||
return { url: pathname, isExact: true, params: {} } | ||
return { url: parent ? parent.url : '/', isExact: true, params: {} } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to change the url
from pathname
to whatever its parent match
's url
is because it otherwise was breaking relative matching. The full location is still available through the location
prop. If someone is using a pathless <Route>
, I'm not even sure why they would be checking the match
.
@@ -44,14 +47,19 @@ const matchPath = (pathname, path, options = {}) => { | |||
if (exact && !isExact) | |||
return null | |||
|
|||
const params = Object.assign({}, | |||
parent && parent.params, | |||
keys.reduce((memo, key, index) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned above, but this merges parent params
and the match's params
into one object. I know that in v2/3, there were two different params objects, so I'm not sure if it would be preferred to return one object with only this path
's params and one with both params merged.
Either I'm just tired, or there's way too much going on in this PR to review all together. Do we need to do this all at once? Or are there pieces we can implement to get closer to where we want to be? |
Most of it is tests, but perhaps that is all the more reason to split this up. I should be able to break this into three parts.
|
Closed in favor or splitting this into multiple parts. Part 1 can be seen here: #4459 |
Re-pulling this for the new v4, if for nothing else than as a proof of concept for relative path support. This adds support for both
<Link>
s (actually I just created a<RelLink>
, but<Link>
could be adapted) as well as<Route>
s and<Switch>
es.This supports relative URL resolving (described by RFC 3986 ) with the difference being that it does not strip off the last URL segment of the
base
pathname (unless there is a trailing slash).This is important because it allows a
<Link>
to be relative to its parentmatch.url
.There are some assumptions made in the resolution process, namely that the
base
pathname is only a pathname (no protocol, domain, search string or hash fragment).I adapted the tests provided by RFC 3986, so I am fairly confident in it, but I'd appreciate breaking tests if I missed any situations.
Quick Example